Skip to content

refactor!: Make Node opaque and optimize it for size#205

Merged
mwcampbell merged 46 commits intomainfrom
opaque-node
Feb 5, 2023
Merged

refactor!: Make Node opaque and optimize it for size#205
mwcampbell merged 46 commits intomainfrom
opaque-node

Conversation

@mwcampbell
Copy link
Contributor

This is a massive breaking change for all users. But I'm afraid it's unavoidable.

This is still a work in progress. The technique is proven to work, but I still need to do the following:

  • Use macros to eliminate as much getter/setter boilerplate as possible
  • Get serialization, deserialization, and JSON schema support working again
  • Use packed bit flags instead of individual bool fields

@mwcampbell
Copy link
Contributor Author

Almost done. I need to implement the JsonSchema trait. Finally, because the base Node struct is now so small (currently 128 bytes), I think it no longer makes sense to box it (as we currently do using Arc).

@mwcampbell
Copy link
Contributor Author

I was previously asked to implement the builder pattern, where the setter methods look like this:

pub fn with_role(mut self, role: Role) -> Self {
    self.role = role;
    self
}

I previously rejected this because I felt it was relying too much on the optimizer to avoid copying the struct around. I also felt that, while the pattern is convenient for hand-written test and demo code, it wouldn't matter much in actual GUI toolkit implementations, and might actually be less convenient in that context. Now I've realized that I was more right with that last point than I knew. In both egui and my Druid proof-of-concept integration, the toolkit stores under-construction Node structs in a collection, and makes mutable references to those structs available for the widget implementations to populate. A move-based builder pattern is incompatible with that implementation. So I'm now sure that my initial instinct was correct. Hand-rolled tests and samples will just have to be a bit more verbose.

@mwcampbell
Copy link
Contributor Author

The change to eliminate the dependency on kurbo may seem to be over-reaching for this PR. But it's part of my plan to make AccessKit acceptable to even the most dependency-conscious projects, and I figured I should make all the planned breaking API changes in the foundational crate at once.

@mwcampbell
Copy link
Contributor Author

This is now ready for review. The impact on a real GUI toolkit can be seen in this egui branch.

@federicomenaquintero Since this is based on one of the techniques that you used to reduce memory usage in librsvg, as we previously discussed in the GNOME accessibility chat room, I'd appreciate your review.

@mwcampbell mwcampbell marked this pull request as ready for review January 15, 2023 14:45
@mwcampbell
Copy link
Contributor Author

There are two things about this branch I'm currently not happy with:

  1. Lack of test coverage. I definitely want to use the serde_test crate to test the Serialize and Deserialize implementations. Some more testing of property-related code paths would also be good.
  2. Mandatory use of a global variable behind a mutex in this foundational crate might not be a good idea, particularly since the mutex is locked with every individual call to NodeBuilder::build. I think I want to replace this with an explicit argument to build, and an optional convenience function to use a global instance of that struct. The Deserialize implementation will have to use the global, though.

Copy link

@federicomenaquintero federicomenaquintero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very interesting; I'm glad that librsvg's optimization also works for you. I learned a couple of tricks from your commits, which may be usable for librsvg (serde magic and such). Thanks for leaving the fine-grained commits around!

($($(#[$doc:meta])* ($base_name:ident, $id:ident, $type_method_base:ident, $getter_result:ty, $setter_param:ty))+) => {
paste! {
impl Node {
$($(#[$doc])*

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kudos for including the docs via macros this way!

Also, I didn't know about the paste crate. This is really useful.

#[cfg_attr(feature = "schemars", derive(JsonSchema))]
#[cfg_attr(feature = "serde", serde(rename_all = "camelCase"))]
#[cfg_attr(feature = "serde", enumset(serialize_as_list))]
enum Flag {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent use of enumset 👌

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, well, I see that you removed it later. No problem, the point was compressing bools to bitflags.

@mwcampbell
Copy link
Contributor Author

I have permission from @DataTriny (my primary collaborator on this project) to go ahead and merge this, though it hasn't yet been fully reviewed. We agree that the change is necessary, I'm confident that the approach is fundamentally sound, and I don't want to hold up corresponding updates in downstream projects. Please open issues or PRs for any problems found in subsequent reviews of this refactor.

@mwcampbell mwcampbell merged commit 4811152 into main Feb 5, 2023
@mwcampbell mwcampbell deleted the opaque-node branch February 5, 2023 15:24
@github-actions github-actions bot mentioned this pull request Feb 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants